Display Header names dynamically - #753
infofromca wants to merge 16 commits into
Conversation
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
e92ec0b to
f6557ef
Compare
|
@sarahelsaig please review it |
|
@sarahelsaig please review it |
|
@sarahelsaig |
|
|
||
| public static class TableHeaders | ||
| { | ||
| public static IList<LocalizedHtmlString> GetDefaultHeaders(IHtmlLocalizer htmlLocalizer, HeadersDisplayNamesOptions options) => |
There was a problem hiding this comment.
Make the first parameter a specific localizer (e.g. IHtmlLocalizer<HeadersDisplayNamesOptions>) to avoid confusion with passing in different translation contexts.
|
|
||
| public static class TableHeaders | ||
| { | ||
| public static IList<LocalizedHtmlString> GetDefaultHeaders(IHtmlLocalizer htmlLocalizer, HeadersDisplayNamesOptions options) => |
There was a problem hiding this comment.
Why is this method called get "default" headers? The values depend on the options, so they aren't default. I think it should be GetLocalizedShoppingCartHeaders.
| <div> | ||
| <strong>@T["Gross Price: {0}", Model.GrossTotal]</strong> | ||
| <strong>@T["{0}: {1}", options.Value.GrossPrice, Model.GrossTotal]</strong> | ||
| </div> |
There was a problem hiding this comment.
The options.Value.NetPrice and options.Value.GrossPrice are not localized here. Add a helper to TableHeaders for these, because they recur in other places as well.
| @T["Net Price: {0}", netTotal] | ||
| @T["{0}: {1}", options.Value.NetPrice, netTotal] | ||
| } | ||
| @if (priceDisplaySettings.UseNetPriceDisplay && priceDisplaySettings.UseGrossPriceDisplay) | ||
| { | ||
| <text>｜</text> | ||
| } | ||
| @if (priceDisplaySettings.UseGrossPriceDisplay) | ||
| { | ||
| @T["Gross Price: {0}", grossTotal] | ||
| @T["{0}: {1}", options.Value.GrossPrice, grossTotal] |
There was a problem hiding this comment.
These should be localized as well.
| <div class="pb-3 field field-type-pricefield field-name-tax-rate-gross-price" | ||
| title="@T["Estimate, the final value is calculated during checkout."]"> | ||
| <strong class="tax-rate-gross-price-title">@T["Gross Price*:"]</strong> | ||
| <strong class="tax-rate-gross-price-title">@T[$"{options.Value.GrossPrice}*:"]</strong> |
There was a problem hiding this comment.
This is invalid. The value of @T[string] must be a compile time constant.
| [RequireFeatures(CommerceConstants.Features.HeadersDisplayNames)] | ||
| public class PriceDisplayNamesStartup : StartupBase | ||
| { | ||
| private readonly IShellConfiguration _configuration; |
There was a problem hiding this comment.
Should be called _shellConfiguration for consistency.
| namespace OrchardCore.Commerce.Abstractions.Extensions; | ||
|
|
||
| public static class TableHeaders |
There was a problem hiding this comment.
The method in this class is not an extension method so this is the wrong place. Also it's more related to localization than tables. So please move this to src/Libraries/OrchardCore.Commerce.Abstractions/Helpers/ and rename the class to LocalizationHelpers.
Co-authored-by: Sára El-Saig <sara.el-saig@lombiq.com>
Co-authored-by: Sára El-Saig <sara.el-saig@lombiq.com>
2 participants
Fix #689
I find a way to show those dynamiclly